Fix copy paste and delete error core paragraph with locking #14712

jorgefilipecosta · 2019-03-29T18:22:45Z

Description

Rich text is able to handle the situation of onReplace and onRemove not existing.
When locking exist onReplace prop is not passed to blocks.
Previously onReplace prop was passed directly passed from paragraph to rich text. Now that's not the case and we have a middle function that may call the onReplace prop. If the onReplace property is not set (e.g we have locking) we should not pass the onReplace to rich text.

How has this been tested?

I added the following CPT with locking:

function gutenberg_test_cpt_locking() {
	register_post_type(
		'locked-all-post',
		array(
			'public'        => true,
			'label'         => 'Locked All Post',
			'show_in_rest'  => true,
			'template'      => $template,
			'template_lock' => 'all',
		)
	);
	register_post_type(
		'locked-insert-post',
		array(
			'public'        => true,
			'label'         => 'Locked Insert Post',
			'show_in_rest'  => true,
			'template'      => $template,
			'template_lock' => 'insert',
		)
	);
}

add_action( 'init', 'gutenberg_test_cpt_locking' );

I verified I can paste in the paragraph.
I verified I can type something and delete all the contents again, and when I press backspace when the paragraph is empty no errors happen.
I repeated the same tests for the paragraph inside the following block:
( function() {
const { registerBlockType } = wp.blocks;
const { createElement: el } = wp.element;
const { InnerBlocks } = wp.editor;

registerBlockType( 'acme/product', {
	title: 'Product',
	icon: 'carrot',
	category: 'common',

	edit() {
		return el( 'div', { className: 'product', style: { outline: '1px solid gray', padding: 5 } },
			// Only paragraphs, images, and products:
			el(
                InnerBlocks,
                { template: [
                    [ 'core/heading', { placeholder: 'Slide heading...', level: 2 } ],
                    [ 'core/paragraph', { placeholder: 'Slide content...' } ],
                    [ 'core/button', { text: 'Read More', className: 'btn-lg' } ],
                ],
                templateLock: 'all'
                }
            )
			// Everything and products:
			//el( InnerBlocks )
		);
	},

	save() {
		return el( 'div', { className: 'product', style: { outline: '1px solid gray', padding: 5 } },
			el( InnerBlocks.Content )
		);
	},
} );

} )();

I verified that If I revert the fix in the paragraph block the tests start failing.

aduth · 2019-04-03T13:28:12Z

When locking exist onReplace prop is not passed to blocks.

Could we address this by tackling #6587 as the same time?

jorgefilipecosta · 2019-04-10T19:35:43Z

Hi @aduth following #6587 would not solve this bug.
RichText would assume onReplace is available when it is not and the copy paste on empty locked paragraphs would not work.
It is possible to simulate that by changing the paragraph code to:

onReplace={ this.props.onReplace ? this.onReplace : () => {} }
onRemove={ this.props.onReplace ? ( () => onReplace( [] ) ) : () => {} }

That said one option I see to go with #6587 while solving this bug without big changes to rich text would be to have a BLOCK_EDITOR_NOOP variable referencing a function that is passed when the operation is not available. RichText would then be able to easily check if the action is not possible by comparing against BLOCK_EDITOR_NOOP.

aduth · 2019-04-29T18:10:34Z

I added the following CPT with locking:

There's an unset variable $template in this snippet.

aduth

copy paste on empty locked paragraphs would not work.

This wasn't clear, but I tested and I see what you mean now: RichText changes its behavior based on whether it receives an onReplace, so it could handle internally to substitute the text when it's not available.

gutenberg/packages/block-editor/src/components/rich-text/index.js

Line 286 in 5b3b3ab

const shouldReplace = this.props.onReplace && this.isEmpty();

Alternatively, we could revise the implementation so that we always pass an onReplace function which is not quite a noop: For a locked template, it would allow a replacement for when the substituted value would still satisfy the template (i.e. replacing one paragraph with another).

That being said, this is still an improvement, and allows us some time to consider whether we want to guarantee a function to be passed always (#6587) separately.

packages/block-library/src/paragraph/edit.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

commit 1dddab4 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Wed May 1 00:00:50 2019 +0300 Have Aztec delete the detected Enter key for paragraphs Aztec-Android doesn't swallow the Enter key (like the list handling does) so, instruct Aztec to delete it for the paragraph block. commit 0936ca0 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Tue Apr 30 20:58:36 2019 +0300 Just use onFormatChange which now defaults to "force" commit 6358de3 Merge: a5282ce 5e4d627 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Tue Apr 30 20:23:57 2019 +0300 Merge branch 'master' into rnmobile/fix-list-handling-android commit 5e4d627 Author: Pascal Birchler <pascal.birchler@gmail.com> Date: Tue Apr 30 18:27:24 2019 +0200 Update to Babel 7.4 and core-js 3 (#15139) * Specify core-js version when using useBuiltIns * Update to Babel 7.4 and core-js 3 * setupFiles: fix path to async-iterator * Also update Babel packages in babel-preset-default * Update snapshot for babel-preset-default test * Manually include web.dom-collections.iterator in jest config * Regenerate package-lock.json file after latest changes applied * Upgrade ESLint related npm packages * Another cleanup in the package-lock.json file * Remove ESLint rules which throw error to fix linting * Add core-js/modules/web.dom.iterable to make e2e tests work * Update Babel packages to the latest version * Update ignore files in e2e tests config * Add changelog entries related to Babel and ESLint version bumps commit 1d959ba Author: Andrés <nosolosw@users.noreply.github.com> Date: Tue Apr 30 17:14:35 2019 +0200 Remove not used state (#15224) commit d715e93 Author: Jon Surrell <jon.surrell@automattic.com> Date: Tue Apr 30 16:29:09 2019 +0200 Babel plugin JSX: Implement Fragment handling (#15120) Add imports for `<></>` JSX Fragments. commit 96cad99 Author: Kelly Dwan <ryelle@users.noreply.github.com> Date: Tue Apr 30 09:18:54 2019 -0400 Fix incorrect ID in FocalPointPicker (#15255) commit c1ba13e Author: Jon Surrell <jon.surrell@automattic.com> Date: Tue Apr 30 13:44:50 2019 +0200 Update Fragment imports in READMEs (#15262) Some Fragment imports were incorrect. Fix them. commit 297c2f4 Author: etoledom <etoledom@icloud.com> Date: Tue Apr 30 13:19:34 2019 +0200 RNMobile: Fix autoscroll on ListBlock (#15048) * RNMobile: Add ability to send extra props (i.e. props needed just for mobile) to the list block. * Adding onCaretVerticalPositionChange to RichText via context * Passing onCaretVerticalPositionChange via context to RichText for all RichText based blocks This makes it not necessary to pass onCaretVerticalPositionChange as a prop directly to RichText from the block component. * Update jest snapshot for block-edit commit 447dd27 Author: Jorge Costa <jorge.costa@developer.pt> Date: Tue Apr 30 10:47:55 2019 +0100 Fix copy paste and delete error core paragraph with locking (#14712) * Fix: Copy / Paste error core/paragraph with locking * Add test case. * Update packages/block-library/src/paragraph/edit.js Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt> commit 0b2eb1c Author: Jon Surrell <jon.surrell@automattic.com> Date: Tue Apr 30 09:39:29 2019 +0200 Dependency extraction webpack plugin: Change option name consistent with docs (#15260) In the docs, an option name was provided as `requestToHandle`, but the actual option in use was `requestToDependency`. Update the code and related tests to use `requestToHandle` as described in the documentation instead of `requestToDependency`. commit 506827e Author: Jon Surrell <jon.surrell@automattic.com> Date: Tue Apr 30 09:31:38 2019 +0200 Read script dependencies from generated files (#15124) * Rework script registration to use generated deps * Deprecate static list of dependencies * Rework translation without packages-dependencies.php * Include build/*/*.deps.json in plugin zip * Add script dependencies undetectable by build tools Some scripts have dependencies that are undetectable by the webpack plugin used to generate the dependency files. Add these dependencies to the generated dependencies. * Add `wp-polyfill` dependency via webpack plugin Enable `injectPolyfill` option in the webpack plugin instead of manually injecting the dependency via PHP on script registration. This will enable the polyfill for consumers of wp-scripts default webpack config. * Improve deprecated message * Remove obsolete lib/packages-dependencies.php commit c2c8276 Author: Daniel Richards <daniel.richards@automattic.com> Date: Tue Apr 30 13:36:49 2019 +0800 Use button block appender on group block (#14943) * Use button block appender on group block * Update group block e2e tests for the new button appender * Add additional margin to the appender when a group has a background color. * Use more opaque versions of the appender background color To ensure greater compatibility when layered on top of a block with a background color. * Block Library: Display ButtonBlockAppender only if selected or empty * Only render button appender when the group has no inner blocks commit 1e96ec7 Author: Andrew Duthie <andrew@andrewduthie.com> Date: Mon Apr 29 16:40:24 2019 -0400 Build Tooling: Pass individual files as arguments from watch to build script (#15219) commit cafb57b Author: Grzegorz (Greg) Ziółkowski <grzegorz.ziolkowski@automattic.com> Date: Mon Apr 29 22:12:08 2019 +0200 Blocks: Upgrade simple-html-tokenizer dependency (#15246) commit 5b3b3ab Author: Andrew Duthie <andrew@andrewduthie.com> Date: Mon Apr 29 13:38:09 2019 -0400 Framework: Add REST API codeowners (#15215) commit 64c48fd Author: Ian Dunn <ian@iandunn.name> Date: Mon Apr 29 10:06:46 2019 -0700 Correct name of `date_i18n()` PHP function. (#15204) commit 8c28814 Author: Andrew Duthie <andrew@andrewduthie.com> Date: Mon Apr 29 13:05:10 2019 -0400 Framework: Remove TESTS.md (#15217) commit 0f1fb57 Author: Emmanuel Hesry <ehesry@gmail.com> Date: Mon Apr 29 17:34:22 2019 +0200 fix typo in withDispatch documentation (#15251) commit a5282ce Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 16:08:31 2019 +0300 Revert "Trivial change to trigger Travis" This reverts commit e22ffde. commit e22ffde Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 16:08:24 2019 +0300 Trivial change to trigger Travis commit 15a51f8 Author: Jorge Costa <jorge.costa@developer.pt> Date: Fri Apr 19 15:33:15 2019 +0100 chore: Fix: Lint error that makes unit tests (and CI tests) fail. (#15073) commit 2b11074 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 12:42:55 2019 +0300 Fix lint issues commit 9c298c8 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 12:01:46 2019 +0300 Need to specify firedAfterTextChanged on all Aztec events commit 46e086d Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 03:59:59 2019 +0300 Force Aztec update if "Enter" fired before text change commit 724e295 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 03:11:52 2019 +0300 Differentiate Android and iOS since assumptions diverged The iOS side still expects to just check against `this.lastContent` to force the change into Aztec. commit 52386fa Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Thu Apr 25 01:55:22 2019 +0300 Use a flag to signal Aztec-originated changes And assume that when that flag is false, component changes need to get sent/reflected down to Aztec. commit c7aa381 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Tue Apr 23 17:30:45 2019 +0300 Able to not lose content commit 60c75b0 Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com> Date: Tue Apr 23 10:09:23 2019 +0300 If text already changed, don't modify it

jorgefilipecosta requested review from aduth, ajitbohra, gziolo, nerrad, noisysocks, notnownikki, ntwb, Soean, talldan and youknowriad as code owners March 29, 2019 18:22

aduth added [Block] Paragraph Affects the Paragraph Block [Type] Bug An existing feature does not function as intended labels Apr 3, 2019

jorgefilipecosta force-pushed the fix/copy-paste-error-core-paragraph-with-locking branch from 1a465b8 to 1ac5c53 Compare April 12, 2019 13:54

aduth approved these changes Apr 29, 2019

View reviewed changes

packages/block-library/src/paragraph/edit.js Outdated Show resolved Hide resolved

aduth mentioned this pull request Apr 29, 2019

Block API: Change block mutation functions from undefined to noop in locked template #6587

Open

jorgefilipecosta and others added 3 commits April 30, 2019 10:30

Fix: Copy / Paste error core/paragraph with locking

a478046

Add test case.

bdd1e0b

Update packages/block-library/src/paragraph/edit.js

6ea624e

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

jorgefilipecosta force-pushed the fix/copy-paste-error-core-paragraph-with-locking branch from 171790d to 6ea624e Compare April 30, 2019 09:35

jorgefilipecosta merged commit 447dd27 into master Apr 30, 2019

jorgefilipecosta deleted the fix/copy-paste-error-core-paragraph-with-locking branch April 30, 2019 09:48

youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copy paste and delete error core paragraph with locking #14712

Fix copy paste and delete error core paragraph with locking #14712

jorgefilipecosta commented Mar 29, 2019

aduth commented Apr 3, 2019

jorgefilipecosta commented Apr 10, 2019

aduth commented Apr 29, 2019

aduth left a comment

Fix copy paste and delete error core paragraph with locking #14712

Fix copy paste and delete error core paragraph with locking #14712

Conversation

jorgefilipecosta commented Mar 29, 2019

Description

How has this been tested?

aduth commented Apr 3, 2019

jorgefilipecosta commented Apr 10, 2019

aduth commented Apr 29, 2019

aduth left a comment

Choose a reason for hiding this comment